enhancement(sources): pre-size HTTP request body buffer from Content-Length#25747
enhancement(sources): pre-size HTTP request body buffer from Content-Length#25747armleth wants to merge 4 commits into
Conversation
github-actions
Bot
added
the
domain: sources
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbf2140b85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let capacity = declared_len | ||
| .and_then(|len| usize::try_from(len).ok()) | ||
| .map_or(0, |len| len.min(max_body_size)); | ||
| let mut bytes = BytesMut::with_capacity(capacity); |
There was a problem hiding this comment.
When an unauthenticated HTTP client sends a Content-Length near the configured limit (100 MiB by default via max_decompressed_size_bytes()) and then stalls or disconnects, this allocates that entire buffer before any body bytes are received. I checked the HTTP source call sites in src/sources/util/http/prelude.rs and src/sources/opentelemetry/http.rs; both use this helper for incoming requests, so many concurrent header-only/slow requests can now reserve large amounts of memory where the previous streaming collector only grew after chunks arrived. Consider capping the initial preallocation to a smaller threshold or growing after the first chunk instead of trusting the header up to the full body limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good point. IMO, it's acceptable given the performance regression that the alternative causes.
What do you guys think about this?
There was a problem hiding this comment.
I'd agree with its suggestion to cap the initial capacity at some suitably large but smaller-than-100MiB number. That would minimize still reallocations, just not eliminate them.
There was a problem hiding this comment.
Done. I set 8 MB somewhat arbitrarily some Vector constants are around these values: Kinesis Firehose: 4 MiB, syslog recv buffer: 4 MiB, Windows event log: 10 MiB), does that seem reasonable?
bruceg
left a comment
bruceg
left a comment
There was a problem hiding this comment.
This makes sense to me, other than the potential restriction on the initial capacity.
| test | ||
| ))] | ||
| async fn collect_body_with_limit<S, B>(body: S, max_body_size: usize) -> Result<Bytes, ErrorMessage> | ||
| const MAX_INITIAL_BODY_CAPACITY: usize = 8 * 1024 * 1024; // 8 MiB |
There was a problem hiding this comment.
edge case: if VECTOR_MAX_DECOMPRESSED_SIZE_BYTES/--max-decompressed-size-bytes is set to a value < 8MiB we should honor it by not allocating more than 32 + max_size + max_size / 6 (this is snappy's worst-case scenario, which is larger than gzip, zstd and zlib's worst-case scenarios)
4 participants
Summary
Follow-up to #25488.
We detected some performance regression that seems to come from #25488. The issue seems to be that we collect the body into a
BytesMutcreated empty, with no pre-defined capacity. As the body streams in, the buffer grows (up to the 100 MiB cap), and each time it needs more room we have to allocate again (and possibly copy all the already-buffered data), as the capacity grows bymax(previous_capacity * 2, requested_size).With this change, we pre-sizes the buffer from the
Content-Lengthheader (capped at the limit), restoring a single-allocation body collection.Change
limited_bodythreads the parsedContent-Lengththrough instead of discarding it.collect_body_with_limitallocatesBytesMut::with_capacity(content_length.min(limit))instead ofBytesMut::new().No behavior change.